feature: Alert users when form has unsaved changes #3346
Conversation
|
Analysis results are not available for those commits View more on Code Climate. |
There was a problem hiding this comment.
This PR is definitely in the right direction.
My wondering developer brain started to think about things.
I'm just going to add things here; feel free to ingore them.
- Would it be difficult to capture which fields are dirty?
- Would it be possible to "undirty" the form if the user changes the field back to the original value
- Would it be possible (or valuable) to show a dirty state (toggleable) through a setting.
All of these kind of converge into the same thing;
to hold previous and new state in JS and compare them.
It brings quite some complexity around it with state that can get dirty from different JS plugins.
But I can't shake that feeling that if I make the form back the way it was before, I'd like the dirty state to go away and not make me anxious of why I need to save the form, or loose my changes (whichever they are).
Do you get this feeling?
Do you think it's just me?
Thanks for the great work.
|
@adrianthedev jumping into the conversation cause I have other concerns regarding what you said; what about custom fields developed by people for their own solutions ? |
|
Yes, I agree it gets deep very quickly. |
|
@adrianthedev I had a fleeting thought on what if the field is reverted back to the original state but forgot about it soon. So, I get what you are talking about. Tracking full state and toggling the form will be way out of scope but un-dirtying should be handled in this PR. I will work on that tomorrow and also start adding tests since this is the intended feature you were looking for. Any tip on where to start to ensure I haven't broken existing tests? Running whole suite would take a lot of time, I assume? |
|
@adrianthedev Form un-dirtying is now implemented. While testing though changes (both dirtying/undirtying) are not being tracked for Markdown fields, I will have to look into the implementation details for Markdowns which will take some time than I had initially anticipated so progress over the days could be slow here. |
|
@adrianthedev Implementation of the code portion is complete. Can you re-review when you have time? I have tested this in possible fields I could find in the app:
|
|
This is still on our plate to review @coolprobn. Sorry for the delay. |
No worries Adrian, I have caught yet another bug in the implementation which interrupts any turbo action and shows the alert box e.g. "modal" or those additional "update" actions that don't move away from the form page. But I haven't been able to work on this after I asked for review as well due to work stuffs, hopefully I will get some time soon again to get back to this. |
|
@adrianthedev This PR is ready to review now from my side. Tests have also been added for the changes. |
coolprobn
changed the title
Description
Add a stimulus controller to track changes in the form and show alert box when user moves away from the current form, either by reloading the page or clicking any other buttons in the application with the use of Turbo Navigation.
Fixes # (issue)
Closes #673
Checklist:
Screenshots & recording
https://www.loom.com/share/9bac6ed248014dfdbcc52d76b1973a99?sid=ca6389a5-eda4-41df-a357-e987a3565233
Manual review steps
Feature walkthrough is in the screen share attached above
TODO